Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add support for APM Agent Configuration via Kibana #1197

Merged
merged 11 commits into from
Jul 29, 2019
Merged

feat: add support for APM Agent Configuration via Kibana #1197

merged 11 commits into from
Jul 29, 2019

Conversation

watson
Copy link
Contributor

@watson watson commented Jul 7, 2019 •
edited by zube bot
Loading

Depends on elastic/apm-nodejs-http-client#66
Closes #1125

Checklist

  • Implement code
  • Add tests
  • Update documentation
  • Update dependency to released version

@watson watson requested a review from Qard July 7, 2019 19:47
@watson watson self-assigned this Jul 7, 2019
@watson
Copy link
Contributor Author

watson commented Jul 7, 2019

The diff might look a bit daunting, but this is mainly due to a refactoring I had to do to ensure the normalize function could be called multiple times (first when the initial config is loaded, and later when we receive new config from the APM Server). If you ignore whitespace it should look a lot simpler.

The issue was that the properties ignoreUrlStr, ignoreUrlRegExp etc all were set on the new config options object every time it was given to normalize. Instead, these are now properties on a new real config object and set in the constructor the first time the config is loaded. And normalize now just expects them to be there.

There's a lot of related refactoring that can be done in the config.js file, but I tried to keep it minimal to not make the PR to confusing.

Qard
Qard reviewed Jul 8, 2019
View reviewed changes
test/remote-config-disabled.js Outdated Show resolved Hide resolved
@watson watson mentioned this pull request Jul 8, 2019
Closed
7 tasks
watson
watson commented Jul 8, 2019
View reviewed changes
docs/configuration.asciidoc Outdated Show resolved Hide resolved
@watson watson requested a review from bmorelli25 July 8, 2019 21:17
bmorelli25
bmorelli25 reviewed Jul 9, 2019
View reviewed changes
Copy link
Member

@bmorelli25 bmorelli25 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking we should also update here: https://github.com/elastic/apm-agent-nodejs/blame/837a63aca1ee7dbf10420690d748d5439d23c89c/docs/setup.asciidoc#L52

Add centralConfig as the # 1 option and link to this documentation. What do you think?

There are four ways to configure the Node.js agent. In order of precedence (higher overwrites lower):

1. APM Agent configuration via Kibana. Enabled with <<central-config>>.

2. Environment variables.

3. If calling the `apm.start()` function,
you can supply a <<agent-configuration-object,configurations object>> as the first argument.

4. Via the <<agent-configuration-file,agent configuration file>>.

@bmorelli25 bmorelli25 mentioned this pull request Jul 9, 2019
Closed
9 tasks
Qard
Qard previously approved these changes Jul 9, 2019
View reviewed changes
watson
watson commented Jul 9, 2019
View reviewed changes
test/config.js
['captureSpanStackTraces', 'CAPTURE_SPAN_STACK_TRACES', true],
['captureBody', 'CAPTURE_BODY', 'off'],
['centralConfig', 'CENTRAL_CONFIG', true],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one of two lines that I added (the diff is a little hard to read as I reordered the array to be alphabetically sorted)

watson
watson commented Jul 9, 2019
View reviewed changes
test/config.js
['kubernetesPodName', 'KUBERNETES_POD_NAME'],
['kubernetesPodUID', 'KUBERNETES_POD_UID'],
['logLevel', 'LOG_LEVEL', 'info'],
['metricsInterval', 'METRICS_INTERVAL', 30],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one of two lines that I added (the diff is a little hard to read as I reordered the array to be alphabetically sorted)

@watson watson requested a review from Qard July 9, 2019 11:40
bmorelli25
bmorelli25 previously approved these changes Jul 9, 2019
View reviewed changes
Copy link
Member

@bmorelli25 bmorelli25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs LGTM

@Qard Qard marked this pull request as ready for review July 29, 2019 03:55
Qard
Qard previously approved these changes Jul 29, 2019
View reviewed changes
@Qard Qard merged commit 2cd65f2 into elastic:master Jul 29, 2019
@watson watson deleted the remote-config branch August 1, 2019 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Qard Qard Qard approved these changes

@bmorelli25 bmorelli25 bmorelli25 left review comments

Assignees

@watson watson

Labels
elastic-7.3 enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add ability to change agent sampleRate from within Kibana
3 participants
@watson @Qard @bmorelli25